Fix SeekLowerBound where tree contains keys that are prefixes of each other #39

banks · 2021-06-16T13:26:19Z

This seems like a trivial case to miss however it stemmed from some confusion (on my part) about whether iradix was ever intended to support keys that were prefixes. It's a long story and I don't even recall exactly why I thought this was the case now given that every other operation on iradix supports this and most even include this case in their test examples. We even had it in the README example!

But some combination of:

I was working at the time on a related radix tree that did have the assumption of null-terminated keys
hashicorp/go-memdb always null terminates even for simpler string indexes

Anyway the fix is simple and the examples now pass.

In addition the fuzz test (which explicitly used the null-termination trick to never trigger this assuming it was necessary in general) now passes without null terminating keys every time I've run it.

… other. This seems like a trivial case to miss howevere it stemmed from some confusion (on my part) about whether iradix was ever intended to support keys that were prefixes. It's a long story and I don't even recall exactly why I thought this was the case now given that every other operation on iradix supports this and most even include this case in their test examples. We even had it in the README example! But some combination of: - I was working at the time on a related radix tree that did have the assumption of null-terminated keys - hashicorp/go-memdb always null terminates even for simpler string indexes Anyway the fix is simple and the examples now pass. In addition the fuzz test (which explicitly used the null-termination trick to never trigger this assuming it was necessary in general) now passes without null terminating keys every time I've run it.

… work too.

ncabatoff

Nice find!

banks · 2021-06-16T14:47:40Z

There are at least a couple of other issues here I'm working on:

there is code that looks like it could cause a panic by referencing search[0] right after a conditional branch that sets search = []byte{}. It turns out that condition is unreachable so I'll remove it.
I missed SeekReverseIterator which has similar issues - I assumed because tests passed it was OK but in fact it only has fuzz tests for lower bound which happened not to provoke this issue any time I ran them. I'm adding table tests and finding that more is needed to not just fix this bad assumption but to also return results in the reversed order!

I'll update this PR when done. Thanks for the review @ncabatoff!

thaJeztah · 2021-06-16T15:41:38Z

iradix_test.go

@@ -1725,17 +1750,15 @@ func TestIterateLowerBoundFuzz(t *testing.T) {
 	set := []string{}

 	// This specifies a property where each call adds a new random key to the radix
-	// tree (with a null byte appended since our tree doesn't support one key
-	// being a prefix of another and treats null bytes specially).
+	// tree.
 	//
 	// It also maintains a plain sorted list of the same set of keys and asserts
 	// that iterating from some random key to the end using LowerBound produces
 	// the same list as filtering all sorted keys that are lower.

 	radixAddAndScan := func(newKey, searchKey readableString) []string {
 		// Append a null byte


Looks like the comment also needs to be removed

oh! think it's removed in the second commit 🤦

tonistiigi · 2021-06-16T20:45:59Z

This seems to fix the issues in buildkit tests moby/buildkit#2182 . Thanks!

Does this also close #35 ?

banks · 2021-06-17T11:18:18Z

Does this also close #35 ?

It should do once I'm done here. Thanks for pointing that out I'd missed it!

Also adds support for SeekReverseLowerBound to work when keys are prefixes of each other.

banks · 2021-06-17T12:37:00Z

I think with the most recent commit this PR now includes:

Support keys that are prefixes of each other in Iterator.SeekLowerBound
Support keys that are prefixes of each other in ReverseIterator.SeekReverseLowerBound
Remove cases that could cause a panic in both SeekLowerBound and SeekReverseLowerBound
Add test examples for both above issues and both directions that provoked the issues reported. Also added a bunch of other examples that were bugs found while reading the code/reasoning about it.
Make all the *LowerBound fuzz tests not append null bytes.

I'd say I'm reasonably confident that this is now more correct than before. I'd not be super surprised if there are still examples that could be found that cause it to break! The many examples I found manually that our fuzz tests have not yet shows that they are certainly not exhaustive currently.

banks · 2021-06-17T13:34:16Z

I've found another panic while tweaking fuzz tests and ensuring full coverage of example tests for both lower bound methods.

I'll add an example and a fix for that.

ncabatoff

Changes are looking great!

iter.go

reverse_iter.go

iradix_test.go

This also uncovered a couple of bugs: - Fuzz tests would break if empty strings are involved (which are valid) due to erroneously "de-duplicating" them - A panic in SeekLowerBound that only occured in specific tree configurations (fixed)

banks · 2021-06-21T21:38:18Z

OK I think those last two commits fix up the remaining issues i've found. I used code coverage reports when running single tests to evaluate the coverage of both the example tests and the fuzz tests to ensure they were exercising every remaining code path in Seek*LowerBound and the associated Next/Previous logic. The fixes include:

Reduce the entropy of Fuzz tests - they were too random before so that it was extremely rare that the keys generated shared much of a prefix and so didn't actually exercise as many of the paths through the logic. Also allow them to include zero length key which found a bunch more issues.
Fix a panic the fuzz tests found in SeekLowerBound in one particular case that the examples didn't cover. There is now an example test that covers that panic too.
Fix several issues in the Fuzz tests that were incorrect when zero-length keys were used
Fix a further bug in SeekReverseLowerBound that caused missed keys on iteration - added an example based on the case found by fuzz tests for this that now passes.
Remove the "non root" fuzz test for SeekReverseLowerBound since coverage suggest the single one now does exercise all relevant cases - the rationale given I think no longer makes sense because the implementation has changed to support internal leaves and to recurse to the max sub-tree during iteration rather than during seek. I couldn't see a case where non-root would inherently exercise paths that root-based tests did not with the right inputs and coverage seems to confirm that.
Ran both remaining fuzz tests at least 20 times each locally without failures.

Thanks for the review @ncabatoff I'll go through your feedback again as I think you had some good points but wanted to get this up today and have this PR in a shape where I think I have some confidence in it's thoroughness again!

banks · 2021-06-22T15:36:01Z

Fuzz tests run 1000 times each locally without a failure after last refactoring:

$ go test -v -run Fuzz -count=1000 | rg '^(---|PASS|ok)'
--- PASS: TestIterateLowerBoundFuzz (0.00s)
--- PASS: TestReverseIterator_SeekReverseLowerBoundFuzz (0.00s)
...
--- PASS: TestIterateLowerBoundFuzz (0.00s)
--- PASS: TestReverseIterator_SeekReverseLowerBoundFuzz (0.00s)
PASS
ok  	github.com/hashicorp/go-immutable-radix	6.527s

reverse_iter.go

lgfa29

The changes look great. Thanks for updating ReverseIterator as well 🎉

thaJeztah · 2021-06-29T20:40:45Z

Thanks!

banks mentioned this pull request Jun 16, 2021

Note the huge prefix/null byte gotcha in README #38

Closed

Clean up other tests that copied the same assumptions and verify they…

3e62349

… work too.

ncabatoff approved these changes Jun 16, 2021

View reviewed changes

thaJeztah reviewed Jun 16, 2021

View reviewed changes

tonistiigi mentioned this pull request Jun 16, 2021

contenthash: use SeekLowerBound to seek radix tree moby/buildkit#2182

Merged

Prevent panics in SeekLowerBound and SeekReverseLowerBound;

5380468

Also adds support for SeekReverseLowerBound to work when keys are prefixes of each other.

banks mentioned this pull request Jun 17, 2021

fix: iterate lower bound may panic if search with the same prefix on keys #35

Closed

Clarify some comments and remove unreachable code

b383238

ncabatoff approved these changes Jun 17, 2021

View reviewed changes

banks added 2 commits June 21, 2021 20:48

Improve fuzz test coverage

16f2cb7

This also uncovered a couple of bugs: - Fuzz tests would break if empty strings are involved (which are valid) due to erroneously "de-duplicating" them - A panic in SeekLowerBound that only occured in specific tree configurations (fixed)

Fix reverse fuzz test and an example it caught that was a bug

8a948c2

banks added 2 commits June 22, 2021 16:25

Remove pointless assignments to i.node

0e888ff

Comment typos and test fixture refactor per PR comments

281dbf7

banks commented Jun 22, 2021

View reviewed changes

reverse_iter.go Outdated Show resolved Hide resolved

Clean up comment and ineffectual assignment

1f03ab4

lgfa29 approved these changes Jun 28, 2021

View reviewed changes

banks merged commit 49d1d02 into master Jun 28, 2021

banks deleted the fix-lower-bound branch June 28, 2021 20:39

thaJeztah mentioned this pull request Jun 29, 2021

add regular seek #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SeekLowerBound where tree contains keys that are prefixes of each other #39

Fix SeekLowerBound where tree contains keys that are prefixes of each other #39

banks commented Jun 16, 2021 •

edited

Loading

ncabatoff left a comment

banks commented Jun 16, 2021

thaJeztah Jun 16, 2021

thaJeztah Jun 16, 2021

tonistiigi commented Jun 16, 2021

banks commented Jun 17, 2021

banks commented Jun 17, 2021

banks commented Jun 17, 2021

ncabatoff left a comment

banks commented Jun 21, 2021 •

edited

Loading

banks commented Jun 22, 2021

lgfa29 left a comment

thaJeztah commented Jun 29, 2021

Fix SeekLowerBound where tree contains keys that are prefixes of each other #39

Fix SeekLowerBound where tree contains keys that are prefixes of each other #39

Conversation

banks commented Jun 16, 2021 • edited Loading

ncabatoff left a comment

Choose a reason for hiding this comment

banks commented Jun 16, 2021

thaJeztah Jun 16, 2021

Choose a reason for hiding this comment

thaJeztah Jun 16, 2021

Choose a reason for hiding this comment

tonistiigi commented Jun 16, 2021

banks commented Jun 17, 2021

banks commented Jun 17, 2021

banks commented Jun 17, 2021

ncabatoff left a comment

Choose a reason for hiding this comment

banks commented Jun 21, 2021 • edited Loading

banks commented Jun 22, 2021

lgfa29 left a comment

Choose a reason for hiding this comment

thaJeztah commented Jun 29, 2021

banks commented Jun 16, 2021 •

edited

Loading

banks commented Jun 21, 2021 •

edited

Loading